Metrics view data conversion#469
Conversation
| for view_data in view_data_list: | ||
| if view_data.view.name == view_name: | ||
| view_data_copy = copy.deepcopy(view_data) | ||
| view_data_copy = copy.copy(view_data) |
There was a problem hiding this comment.
@songy23 I'm assuming the deepcopy here was defensive, so the returned ViewData had a separate _tag_value_aggregation_data_map. But this is also creating new Views (and associated measures and aggregations) on each call, which I don't think you want.
Changing this to copy means that the "copies" returned here share the aggregation data map. A better solution might be to implement ViewData.__copy__ or come up with a new immutable view data obj for export.
There was a problem hiding this comment.
I'm assuming the deepcopy here was defensive, so the returned ViewData had a separate _tag_value_aggregation_data_map.
That's correct.
Changing this to copy means that the "copies" returned here share the aggregation data map.
Ideally the ViewData returned should be immutable, i.e:
vd1 = stats.get_view("some view") # vd1 has data 1.0
vd1.record(1.0) # vd1 now has data 2.0
vd2 = stats.get_view("some view") # vd2 should still have data 1.0So at least the _tag_value_aggregation_data_map should be deeply copied.
A better solution might be to implement ViewData.copy or come up with a new immutable view data obj for export.
+1 for the latter (using immutable view data obj for export). This is exactly what we do in Java.
|
|
||
| # Cache the converted MetricDescriptor here to avoid creating it each | ||
| # time we convert a ViewData that realizes this View into a Metric. | ||
| self._md_cache_lock = threading.Lock() |
There was a problem hiding this comment.
Other stats classes are generally not threadsafe. I don't know that it's worth opening this can of worms in this PR.
c24t
force-pushed
the
metrics-viewdata-conversion
branch
from
601146f to
c8db0ad
Compare
c24t
force-pushed
the
metrics-viewdata-conversion
branch
from
c8db0ad to
e9e0d39
Compare
| if view_data.view.name == view_name: | ||
| break | ||
| else: | ||
| return None |
There was a problem hiding this comment.
I rewrote this for clarity, the only behavior change is the copy below. Note that we're not using timestamp here, there are likely other bugs hiding in this class.
5 participants
This PR adds a utility function to convert
ViewDatas intoMetrics, following up on #467.@mayurkale22 I'm sending this your way for early review. We could merge this change safely by itself, or extend it to include:
Edit: done:
Addresses #335.